Feat/add address for user - #21
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)✅ Unit Test PR creation complete.
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Note Unit test generation is an Early Access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed everything up to 74c7a7e in 2 minutes and 27 seconds. Click for details.
- Reviewed
3055lines of code in33files - Skipped
0files when reviewing. - Skipped posting
12draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src/main/java/iuh/fit/se/ecommerce/service/impl/ProductServiceImpl.java:368
- Draft comment:
The natural-language parsing for search criteria uses multiple nested regex with empty catch blocks. Consider logging or handling parse exceptions instead of silently ignoring them for easier debugging. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment is about code quality - suggesting to log exceptions instead of silently ignoring them. This is generally good advice. However, I need to consider: 1) The same pattern already exists in the unchanged findByQuery method, so this isn't a new issue introduced by this PR. 2) The buildSearchCriteria method is new code added in this PR, so technically it is about changes. 3) The comment is a code quality suggestion that is actionable and clear. 4) However, the pattern of ignoring parse exceptions in search query parsing is a deliberate design choice - it allows the search to gracefully degrade when regex parsing fails, falling back to other search mechanisms. 5) The comment doesn't point out a bug, just suggests a different approach to error handling that may or may not be appropriate for this use case. While the buildSearchCriteria method is new, the exact same exception handling pattern exists in the pre-existing findByQuery method, suggesting this is an intentional design pattern in this codebase. The comment is suggesting a refactor rather than pointing out a clear issue. Additionally, for search query parsing, silently ignoring malformed input and falling back to other search strategies is often the correct behavior - logging every failed parse attempt could create noise. Even though this pattern exists elsewhere, the buildSearchCriteria method is new code being added in this PR. However, the comment is more of a "nice to have" code quality suggestion rather than identifying a clear problem. The silent failure is likely intentional for graceful degradation in search functionality. Without evidence that this is causing actual issues, this is speculative improvement advice rather than actionable feedback on a real problem. This comment should be deleted. While it's about new code, it's suggesting a code quality improvement that may not be appropriate for this use case. The silent exception handling appears to be an intentional design choice for graceful degradation in search parsing, and the same pattern exists in the unchanged code. The comment doesn't identify a clear bug or issue, just suggests a different approach that may not be better.
2. src/main/java/iuh/fit/se/ecommerce/service/interfaces/AddressService.java:1
- Draft comment:
The AddressService interface looks straightforward. Ensure that implementing classes perform proper input validation and error handling. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%None
3. src/main/java/iuh/fit/se/ecommerce/service/interfaces/NominatimService.java:1
- Draft comment:
Interface is simple; ensure that implementations correctly handle API rate limiting and error cases. - Reason this comment was not posted:
Confidence changes required:20%<= threshold50%None
4. src/main/resources/static/js/address-form.js:5
- Draft comment:
Avoid using 'var' for global declaration; prefer 'let' or 'const' consistently to prevent potential scoping issues. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% This is a code quality/refactoring suggestion about using modern JavaScript variable declarations. The comment is technically correct - usingvarcan cause scoping issues and modern JavaScript preferslet/const. However, I need to consider: 1) The use ofvarhere is intentional because it's creating a global variable (function-scoped/global-scoped), and the comment on line 4 explicitly states "userAddresses sẽ được set từ checkout.js hoặc profile.html" suggesting this is meant to be shared across files. 2) Usingletinside theifblock would create a block-scoped variable, which might not work as intended for cross-file sharing. 3) The rules state "Comments that suggest code quality refactors are good! But only if they are actionable and clear." This is actionable and clear. 4) However, the comment doesn't acknowledge the intentional global scope needed here, so it might actually break the functionality if followed blindly. The use ofvarhere appears intentional for creating a global variable that can be accessed across different JavaScript files (checkout.js, profile.html). Simply changing toletwould create block scope which might break the intended cross-file variable sharing. The comment doesn't acknowledge this nuance and could lead to a breaking change if followed without understanding the context. While the comment is technically about better practices, it fails to recognize thatvarmight be intentionally used here for global scope. The comment as written could mislead the developer into making a change that breaks functionality. A better comment would acknowledge the global scope requirement and suggest alternatives like explicitly assigning towindow.userAddressesif global scope is needed. This comment should be deleted. While it's a code quality suggestion, it doesn't account for the apparent intentional use ofvarfor global scope sharing across files. The comment could lead to a breaking change if followed without deeper understanding, making it potentially harmful rather than helpful.
5. src/main/resources/static/js/map-picker.js:86
- Draft comment:
The reverseGeocode function uses a global window.lastGeocodeTime for throttling. If multiple map pickers are ever used concurrently, consider scoping the throttler per instance. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% The comment is speculative - it says "if multiple map pickers are ever used concurrently, consider..." This is a hypothetical future scenario. The current code has anisInitializedflag that prevents multiple map instances from being created. The comment is asking the author to consider a scenario that doesn't currently exist in the code. According to the rules, I should NOT make speculative comments like "If X, then Y is an issue" - I should only comment if it's definitely an issue. This comment is clearly speculative ("if multiple map pickers are ever used") and is not about a definite problem in the current code. However, the code does have a design issue where all the state is global/module-level, and theisInitializedflag suggests the author may have considered multiple instances. Perhaps this is a legitimate architectural concern that should be addressed now rather than later? Even if there's a potential architectural concern, the comment is still speculative about a future use case ("if multiple map pickers are ever used"). The rules explicitly state to NOT make speculative comments. The current code works correctly for its current design (singleton pattern). If the author wanted to support multiple instances, they would need to refactor the entire module, not just the throttling. This comment should be deleted because it's speculative about a future scenario ("if multiple map pickers are ever used concurrently") rather than addressing a definite issue with the current code. The rules explicitly prohibit speculative comments.
6. src/main/resources/static/js/api.js:28
- Draft comment:
Storing tokens in localStorage can be vulnerable to XSS attacks; consider secure alternatives like HTTP-only cookies if possible. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. src/main/resources/static/js/search-results.js:155
- Draft comment:
Pagination logic uses conditional ellipsis; double-check edge cases to ensure that ellipses are rendered without duplication. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to double-check edge cases, which violates the rule against asking for confirmation or double-checking. It doesn't provide a specific suggestion or point out a clear issue with the code.
8. src/main/resources/templates/user/profile.html:147
- Draft comment:
In the Addresses tab, ensure that user inputs are properly sanitized on both client and server to prevent XSS vulnerabilities. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%This comment is asking the PR author to ensure that user inputs are sanitized to prevent XSS vulnerabilities. It is not making a specific suggestion or pointing out a specific issue in the code. It is more of a general reminder or request for confirmation, which violates the rule against asking the author to ensure behavior is intended or tested.
9. src/main/resources/static/js/checkout.js:4
- Draft comment:
Ensure that the checkout page only loads over HTTPS to protect sensitive payment data. Consider adding a redirect if not secure. - Reason this comment was not posted:
Comment was on unchanged code.
10. src/main/resources/templates/checkout/checkout.html:69
- Draft comment:
The checkout form is well structured. Just ensure proper ARIA attributes are added for accessibility and confirm that all sensitive data is handled via secure channels. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to ensure that ARIA attributes are added and that sensitive data is handled securely. This is a general request for confirmation and does not provide a specific code suggestion or point out a specific issue in the code. It violates the rule against asking the author to confirm or ensure behavior without being specific.
11. src/main/resources/templates/search-results.html:24
- Draft comment:
The search results page is clear. Consider adding alt text for any images and ARIA roles to improve accessibility. - Reason this comment was not posted:
Comment was on unchanged code.
12. src/main/resources/templates/user/profile.html:60
- Draft comment:
User profile page: Ensure that any dynamic content (e.g. addresses) is also validated on the server side to prevent injection attacks. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_ZNMXPYlvizpZK0XH
You can customize 
| criteria.setProductType("ACCESSORY"); | ||
|
|
||
| // price patterns | ||
| try { |
There was a problem hiding this comment.
Consider adding inline comments to clarify the intent of each regex for price parsing to improve maintainability.
|
✅ UTG Post-Process Complete No new issues were detected in the generated code and all check runs have completed. The unit test generation process has completed successfully. |
|
Creating a PR to put the unit tests in... The changes have been created in this pull request: View PR |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Important
Adds user address management with geolocation, enhances checkout and search functionalities, and updates UI for address handling.
AddressControllerfor CRUD operations on user addresses.AddressServiceandAddressServiceImplfor address logic.AddressRequestandAddressResponseDTOs.Addressentity to include latitude, longitude, and timestamps.AddressRepositorymethods for user-specific queries.GeocodingControllerfor reverse geocoding.NominatimServiceandNominatimServiceImplfor geocoding logic.NominatimCacheKeyGeneratorfor caching geocode results.checkout.jsto handle address creation and selection.checkout.htmlto include address fields and map integration.search-results.js.search-results.htmlfor new search features.profile.html.map-picker.js.This description was created by
for 74c7a7e. You can customize this summary. It will automatically update as commits are pushed.